-
Notifications
You must be signed in to change notification settings - Fork 70
feat(cloudflare): add optional R2 batch uploads via rclone for cache population #925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(cloudflare): add optional R2 batch uploads via rclone for cache population #925
Conversation
🦋 Changeset detectedLatest commit: fe8f553 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@krzysztof-palka-monogo thank you so much for this PR, it will please many of our users! I only took a quick look and I'll do a full review on Monday. I have some preliminary comments/thoughts:
|
Good morning @vicb Here’s the justification for keeping an explicit
|
We definitely don't want "rclone" in the flag. We could use another mechanism, i.e. remote bindings.
IMO we should drop the need for an external config in favor of using env vars - the CLI could dynamically create a temp config file for rclone.js as it doesn't seem to support env vars.
We can use rclone when configured and fallback to the current mechanism - but log that there is faster way and link some docs.
I disagree here. |
Hi @vicb I've updated the PR to align with your suggestions:
Documentation now focuses on "batch upload" as an optional performance feature, not implementation details. Please let me know if thats will meet the requirements and if so could we run "Publish prereleases" action to properly test the changes in some real applications 😄 |
commit: |
Hi @vicb I will be glad for code review. |
function createTempRcloneConfig(): string | null { | ||
const accessKey = process.env.R2_ACCESS_KEY_ID; | ||
const secretKey = process.env.R2_SECRET_ACCESS_KEY; | ||
const accountId = process.env.R2_ACCOUNT_ID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid double access, maybe there could be a retrieveR2CredentialsFromEnv
returning {R2_ACCESS_KEY_ID, R2_SECRET_ACCESS_KEY, CLOUDFLARE_ACCOUNT_ID}
(I don't think the account as anything to do with R2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note sure I'll have time for a full review today, sorry.
A couple comments:
-
R2_ACCOUNT_ID
should rather beCLOUDFLARE_ACCOUNT_ID
as it is not specific to R2 -
Thanks for implementing the env vars based solution for CI. I'm wondering if we can improve the story for local dev. It would be nice to get the vars from
.env
/.dev.vars
files but it would mean adding vars that are only used in local dev and that might confuse users. Maybe we could look for a local<pjt>/rclone.conf
and fallback to env vars when it's not here. What do you think?
Hello @vicb I've addressed both points:
This keeps the implementation simple while making local dev more convenient. WDYT? |
try { | ||
// Cleanup temporary config file | ||
rmSync(tempConfigPath); | ||
} catch { | ||
console.warn(`Failed to remove temporary config at ${tempConfigPath}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use force and recursive instead of try/catch?
I don't think there is an ideal solution for That being said, Next.js uses .env files with this loading order so we should make sure we are consistent. I'll take a deeper look today and comment on how we could do that |
I’ll be away for a week starting tomorrow, so I need to wrap up this functionality today 😅 |
Batch upload R2 cache using rclone
This PR proposes adding support for optional batch uploading R2 cache using rclone.
Based on the solution discussed in Issue #866
Details
Automatic selection between batch vs standard upload
The CLI now detects when all of
R2_ACCESS_KEY_ID
,R2_SECRET_ACCESS_KEY
, andCLOUDFLARE_ACCOUNT_ID
are set in the environment and switches to batch upload mode. Otherwise, it falls back to the standard (Wrangler-based) upload method.Graceful fallback on errors
If batch upload fails (e.g. due to rclone errors), the flow will log a warning and transparently revert to the default upload approach.
Staging and parallel transfers
Assets are copied into a temporary staging directory, then uploaded using rclone copy with concurrency settings (
--transfers=32
,--checkers=16
) for performance.Related issues